-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(lib): use inject to pass container reference to selectableItem #112
base: master
Are you sure you want to change the base?
Conversation
Hey @bvkimball, thanks for your PR. This is actually something I was fiddling with as well at some point but it never made it into master. I like this refactoring and agree with what you outlined above. I am definitely down to adopting this PR if you:
How does that sound? Thanks for your contribution 👍 and I am glad to hear you like this library and it's useful to you. |
Awesome! I will try to get this cleaned up over the weekend. |
Love it. Thanks a bunch! |
refactor: removed dead code from change and fixed circular dependency caused by DI
Not perfect yet but I think tests are working again. Issue caused because the Parent-Child Injection causes circular dependency. The demo is a bit contrived but it is similar to the usecase I am using the library for. I intend to use it with the CDK DragAndDropModule, so that what the demo does. Here is a gif of the demo. When I re-order the tasks then try to use the select box it selects the task based on the previous position, i tried running selectionContainer.update() but that didn't fix the issue. I am trying to make sure this isn't something i caused with the change or the demo code itself. Another issue I am looking at is that click to select doesn't seem to work, i think that might be a conflict with Material or the Drag&Drop Module too. |
Yep that demo is looking slick! Thanks for this. Will be a great addition. Make sure to not break existing functionality. As to the "click to select", I am not sure. Could be indeed a conflict with the Drag & Drop module. |
Let me know when this PR is ready for review and I ll look at it. Maybe you can also take the list of action items I posted earlier and make a small todo list, so we keep track of what's done. |
test(app): added cypress test for new example demo
I think everything is done and working, and I add docs and some cypress test. I believe there are no breaking changes unless someone was using $selectItems which is the QueryList that was removed. that was public but really should have been used for Internal use only.... |
Yep, agreed that the QueryList shouldn't have been public in the first place. If it causes a breaking change we can revert this and publish this as a new major version. I ll look at your changes today and review everything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already looking really good. I love this addition / change.
Left a few remarks though.
Also, can you change dragndrop
to be drag-and-drop
?
Thanks for this contribution!
@@ -1,5 +1,7 @@ | |||
import { ComponentType } from '@angular/cdk/portal'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not use this, because the CDK is not a peer dependency of this library and IMO we shouldn't use anything from the CDK for the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed this was silly of me, I just created a version of ComponentType in the models.ts, it is just a Util type anyway that wraps a interface in a constructable, hope this works.
projects/ngx-drag-to-select/src/lib/select-container.component.ts
Outdated
Show resolved
Hide resolved
docs: updated grammar
I think i have updated everything correctly, sorry about my poor grammar skills ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few minor remarks, most of which were comments from my previous PR which have not been updated. Not sure if the PR was not entirely updated or if you just forgot a few comments. Not a biggy tho.
it('should drag to new list', () => { | ||
getDragAndDropExample().within(() => { | ||
getTodoList() | ||
// Select First 3 items in list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you forget this?
.getSelectItem(2) | ||
.dispatch('mousemove') | ||
.dispatch('mouseup') | ||
// Click on second item |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here too.
// Click on second item | ||
.getSelectItem(1) | ||
.dispatch('mousedown', { button: 0 }) | ||
// Drag to Select Item in other list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same again.
style: update whitespace
ok, sorry about those missed comments, I wish GitHub had an easy way to view them in a list. I resolved the old outdated comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good now! Thanks a lot for this PR. I think it's a great addition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to mention that I'd like to move the drag & drop demo above the mobile demo.
Also, I found some more cosmetic issues. I know, that sucks but I cannot really automatically enforce newlines in scss files.
Can you again explain why range selection and CMD click doesn't work anymore with the drag and drop demo? Is that because Angular Material intercepts the click?
style(app): linting some scss
No worries, updated. I also added the Yeah the click to select functionality doesn't work because of the It would be awesome if the |
I guess a solution could be to pass mouse-events from the selectItem as a "CustomEvent" or pass through to the parent via the reference we just added via DI. |
Yea ok, that makes sense. Then I guess we can ignore this for now. It's fine because it's mostly related to the CDK then. Tests are failing now. I think you have to update your tests to account for the desktop / mobile change. |
Yeah, Cypress error was because the "Item" animates while you drag to reorder, and because the demo isn't at the bottom of the screen anymore the |
ugh... yeah, i see it, it is caused because i hide |
Awesome! Thanks. Looking forward to that fix. Then we are good to go I think. |
Ok this fix is not great... Its kindof a hack, but since the |
f737d7d
to
1fd7215
Compare
I just come here when trying to use Drag & Drop with this lib, and I wonder if this function works? |
Needed to use
[dtsSelectItem]
in grandchildren components which can't be found@ContentChildren
Now using dependency injection i pass the reference of the
SelectContainerComponent
to anySelectItemDirective
that is a child of that container regardless of how deep in the tree the component is.This is similar to how material does this for drag and drop components for the CDK. see here
Just pushing this out to see if there is interest in adopting this change, currently the demo app work as is, but I can update the demo app with an example of nested components using the directive and do a little more cleanup.